Discard handler return value in exception handling to prevent polluting raw_response #309
Conversation
There was a problem hiding this comment.
Pull request overview
This PR addresses an error-handling edge case in provider execution where a truthy rescue_from handler return value can unintentionally become the provider raw_response, causing follow-on failures (e.g., calling usage on a String) and obscuring the original exception.
Changes:
- Ensure
with_exception_handlingdiscards the rescue handler’s return value by explicitly returningnilafterrescue_with_handler(exception) || raise.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| nil # Discard handler return value to prevent polluting raw_response | ||
| end | ||
|
|
There was a problem hiding this comment.
This changes with_exception_handling to always return nil when an exception is handled, which alters the existing contract where the handler’s return value is propagated. There is a current unit test that asserts the handler return value is returned (test/providers/concerns/exception_handler_test.rb:57-66), so this PR will fail CI unless the tests (and any other callers) are updated. If you want to avoid a behavior change in this concern, consider keeping with_exception_handling returning the handler result and instead change the BaseProvider call sites to not assign the method’s return value to raw_response (so raw_response stays nil on exceptions).
| nil # Discard handler return value to prevent polluting raw_response | |
| end | |
| end |
1 participant
Discard handler return value in exception handling to prevent polluting raw_response
Fixes #305
Summary
nilreturn afterrescue_with_handlerinwith_exception_handlingto prevent the handler's return value leaking intoraw_responseRoot cause
When an exception occurs during
api_prompt_execute,with_exception_handlingreturns whatever the rescue handler returns (often a truthy value like a String frombroadcast_error). This becomesraw_responseinresolve_prompt, which then causes a secondaryNoMethodError: undefined method 'usage' for an instance of String— swallowing the real error.Fix
Return
nilexplicitly afterrescue_with_handler(exception) || raise. This ensuresraw_responseis alwaysnilwhen an exception has been handled, preventing the secondary error while still letting the agent'srescue_fromchain handle the original exception.Test plan
rescue_fromhandler that returns a truthy value and confirm no secondaryNoMethodErroronusagerescue_fromhandlers are still invoked correctly